breaking: Switch to ST124 "shorthand notation" syntax in charmcraft.yaml #258
+292
−193
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Will not be merged until coordinated charmcraft 3 migration (https://chat.canonical.com/canonical/pl/wekd7r4eqtdn7mxhgxgej8zzsr) Depends on #258 |
carlcsaposs-canonical
requested review from
paulomach,
taurus-forever,
shayancanonical and
sinclert-canonical
Comment on lines
9
to
10
| # TODO: use permalink | ||
| warnings.warn( |
There was a problem hiding this comment.
will address right before merging (when version number for permalink can be predicted)
carlcsaposs-canonical
changed the title
carlcsaposs-canonical
force-pushed
the
charmcraft-3
branch
from
b18ac77 to
6fbbae5
Compare
carlcsaposs-canonical
force-pushed
the
charmcraftst124
branch
from
cd58963 to
941e2de
Compare
Batalex
approved these changes
Jan 9, 2025
carlcsaposs-canonical
removed request for
paulomach,
shayancanonical and
sinclert-canonical
python/cli/data_platform_workflows_cli/craft_tools/charmcraft_platforms.py
Show resolved
Hide resolved
Comment on lines
+62
to
+69
| if not isinstance(platforms_, dict): | ||
| raise TypeError("Expected type 'dict' for snapcraft.yaml 'platforms'") | ||
| for value in platforms_.values(): | ||
| if value is not None: | ||
| raise ValueError( | ||
| "Only shorthand notation supported in snapcraft.yaml 'platforms'. " | ||
| "'build-on' and 'build-for' not supported" | ||
| ) |
There was a problem hiding this comment.
I can't help but think that this is a validation snippet, which could be extracted to a helper function to reduce the cognitive load of parsing this whole collect function. WDYT?
There was a problem hiding this comment.
I don't think adding a layer of abstraction would improve readability
There was a problem hiding this comment.
I would not call replacing a snippet by a function call adding an abstraction layer. Just in case I did not explain myself well enough, this is what I am proposing:
Before
if yaml_data["base"] == "core24":
platforms_ = yaml_data["platforms"]
if not isinstance(platforms_, dict):
raise TypeError("Expected type 'dict' for snapcraft.yaml 'platforms'")
for value in platforms_.values():
if value is not None:
raise ValueError(
"Only shorthand notation supported in snapcraft.yaml 'platforms'. "
"'build-on' and 'build-for' not supported"
)
for platform in platforms_:
# Example `platform`: "amd64"
architecture = craft.Architecture(platform)
platforms.append({"name": platform, "runner": RUNNERS[architecture]})
elif yaml_data["base"] == "core22":
for entry in yaml_data["architectures"]:
# Example: "amd64"
platform = entry["build-on"]
architecture = craft.Architecture(platform)
platforms.append({"name": platform, "runner": RUNNERS[architecture]})After
if yaml_data["base"] == "core24":
validate_snap_platforms(yaml_data["platforms"]) # However you want to call it
for platform in yaml_data["platforms"]:
# Example `platform`: "amd64"
architecture = craft.Architecture(platform)
platforms.append({"name": platform, "runner": RUNNERS[architecture]})
elif yaml_data["base"] == "core22":
for entry in yaml_data["architectures"]:
# Example: "amd64"
platform = entry["build-on"]
architecture = craft.Architecture(platform)
platforms.append({"name": platform, "runner": RUNNERS[architecture]})
taurus-forever
approved these changes
Jan 9, 2025
Enables building & releasing multi-base charms with 24.04 in a single charmcraft.yaml and git branch Integration testing is not supported on multiple bases—it is currently only supported on 22.04
Adds support for core24 `platforms` in snapcraft.yaml
carlcsaposs-canonical
force-pushed
the
charmcraftst124
branch
from
cbde588 to
8b74ad4
Compare
This reverts commit 8b74ad4.
carlcsaposs-canonical
added a commit
that referenced
this pull request
Jan 16, 2025
carlcsaposs-canonical
added a commit
that referenced
this pull request
Jan 16, 2025
carlcsaposs-canonical
added a commit
that referenced
this pull request
Jan 17, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enables building & releasing multi-base charms with 24.04 in a single charmcraft.yaml and git branch
Integration testing is not supported on multiple bases—it is currently only supported on 22.04
Breaking changes
pytest-operator-cache(which overridesops_test.build_charmfrom pytest-operator) now assumes 22.04 baseDeprecation notice
pytest-operator-cacheis deprecated & will be removed in a future release